uefi: Drop the panic-on-logger-errors feature #1143
Merged
+9
−26
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was originally added because of occasional panics when logging quickly on VirtualBox. (Unclear whether this bug is still present, and also as far as I know we haven't observed this behavior on any other UEFI implementations.) The decision was made to panic by default on logger errors, but offer an escape mechanism. However, making this a compile-time choice is not ideal, since most UEFI programs are intended to run on arbitrary UEFI implementations. We could make it a runtime option instead, but since loggers are usually just informational (i.e. not critical functionality for the application), silently ignoring errors seems like a better choice for most uses. In the rare case where an application does consider logging critical, they can turn off the `logger` helper and implement their own logger. For prior discussion, see: * rust-osdev#121 * rust-osdev#123
phip1611
reviewed
Apr 21, 2024
| // Ignore all errors. Since we're in the logger implementation we | ||
| // can't log the error. We also don't want to panic, since logging | ||
| // is generally not critical functionality. | ||
| let _ = DecoratedLog::write( |
There was a problem hiding this comment.
nit: We could write it to the serial device and the debugcon device in that case.
phip1611
approved these changes
Apr 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was originally added because of occasional panics when logging quickly on VirtualBox. (Unclear whether this bug is still present, and also as far as I know we haven't observed this behavior on any other UEFI implementations.) The decision was made to panic by default on logger errors, but offer an escape mechanism. However, making this a compile-time choice is not ideal, since most UEFI programs are intended to run on arbitrary UEFI implementations.
We could make it a runtime option instead, but since loggers are usually just informational (i.e. not critical functionality for the application), silently ignoring errors seems like a better choice for most uses.
In the rare case where an application does consider logging critical, they can turn off the
loggerhelper and implement their own logger.For prior discussion, see:
Checklist